Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

borrowck: consolidate mut suggestions #40841

Merged
merged 2 commits into from
Mar 29, 2017
Merged

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Mar 26, 2017

This converts all of borrowck's mut suggestions to a new
mc::ImmutabilityBlame API instead of the current mix of various hacks.

Fixes #35937.
Fixes #40823.
Fixes #40859.

cc @estebank
r? @pnkfelix

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it (besides a couple of nitpicks).

x: X
}

fn main() {
pub fn main() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pub fn main?

}

fn local_ty(&self, node_id: ast::NodeId) -> Option<&hir::Ty> {
let parent = self.tcx.hir.get_parent_node(node_id);
let parent_node = self.tcx.hir.get(parent);

// The parent node is like a fn
if let Some(fn_like) = FnLikeNode::from_node(parent_node) {
// `nid`'s parent's `Body`
let fn_body = self.tcx.hir.body(fn_like.body());
// Get the position of `nid` in the arguments list
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update comment to node_id

if let Ok(snippet) = self.tcx.sess.codemap().span_to_snippet(let_span) {
if self.tcx.hir.name(node_id) == keywords::SelfValue.name() &&
snippet != "self" {
// avoid suggesting `mut &self`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it suggest &mut self in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. Binding vs. reference - the code is intentionally careful to do the right thing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, you're right, of course.

@@ -198,7 +198,7 @@ fn main() {
```
"##,

E0386: r##"
/*E0386: r##"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the procedure to remove Errors that we no longer emit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's no such procedure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We put these into register_diagnostics! {}, commented out, although a better way to annotate codes that may not be reused should be made.

Copy link
Member

@pnkfelix pnkfelix Mar 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nagisa are you saying that this modification (the commenting out here) is okay as is? Or are you saying it shouldn't stay here under register_long_diagnostics!, and should ... move to register_diagnostics!...?

I think you must mean the former, that this is okay as is... Let us know if I misunderstand.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(well I guess deleting all this long commented-out diagnostic text and replacing it with one line in register_diagnostics! could be an attractive option...)

@ghost
Copy link

ghost commented Mar 26, 2017

Is #35927 supposed to link somewhere else?

arielb1 added 2 commits March 27, 2017 01:37
This converts all of borrowck's `mut` suggestions to a new
`mc::ImmutabilityBlame` API instead of the current mix of various hacks.

Fixes rust-lang#35937.
Fixes rust-lang#40823.
@arielb1
Copy link
Contributor Author

arielb1 commented Mar 26, 2017

@whataloadofwhat

Indeed. Updated & fixed.

BTW, @eddyb & @petrochenkov - is there a cleaner way of determining whether a function has an implicit self?

@eddyb
Copy link
Member

eddyb commented Mar 27, 2017

@arielb1 Doubt it. It might be better idea to preserve the distinction in hir::Ty instead, not sure.

@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 29, 2017

📌 Commit 39011f8 has been approved by pnkfelix

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 29, 2017
borrowck: consolidate `mut` suggestions

This converts all of borrowck's `mut` suggestions to a new
`mc::ImmutabilityBlame` API instead of the current mix of various hacks.

Fixes rust-lang#35937.
Fixes rust-lang#40823.
Fixes rust-lang#40859.

cc @estebank
r? @pnkfelix
bors added a commit that referenced this pull request Mar 29, 2017
Rollup of 5 pull requests

- Successful merges: #40720, #40786, #40841, #40866, #40897
- Failed merges:
@bors bors merged commit 39011f8 into rust-lang:master Mar 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants